Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

genai: Fix handling of optional arrays in tool input #661

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

piotr-rudnik
Copy link

PR Description

This code fixes handling of optional arrays in pydantic models provided as tool inputs like this:

class ExampleToolInput(BaseModel):
        numbers: Optional[List[str]] = Field()

Optional parameters are handled by setting nullable field in gemini API. Current code works for every type, except for arrays, because items field, that contains information about type of values inside the array, is present in anyOf field as one of the possibilities, where it's ignored.

Also added log where user is warned that only only type can be used inside array. Before, last defined type was used.
This is an edge case where user could be trying to use an array with multiple types which is not supported by genai, but can be useful for finding bugs

Type

🐛 Bug Fix
✅ Test

Testing(optional)

Launching pytest with arguments test_function_utils.py::test_tool_input_can_have_optional_arrays --no-header --no-summary -q in /xxx/langchain-google/libs/genai/tests/unit_tests

============================= test session starts ==============================
collecting ... collected 1 item

test_function_utils.py::test_tool_input_can_have_optional_arrays 

========================= 1 passed, 1 warning in 0.61s =========================

@Shahar-Y
Copy link
Contributor

Shahar-Y commented Dec 22, 2024

Hi, great catch!
Notice we already solved this bug in #658 a few day ago as a fix for #657, hopefully will be merged into main soon 😄

@piotr-rudnik
Copy link
Author

Hey @Shahar-Y, I saw your PR before and it's similar but not the same, tried your code and didn't work in my case. Here, arrays are handled and they have items field setup, not properties in v object (I'm not that invested in the codebase, maybe it's not a problem). But those cases are very similar, maybe could be merged somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants